Bug 1995464 - Overhaul guided bug entry form - #2551
Conversation
| if (_component) { | ||
| params.set('component', _component); | ||
| } | ||
| const noSetHistory = !window.history.state; |
There was a problem hiding this comment.
Blocking — this condition is inverted, and it breaks all deep links.
master had if (!window.history.state) { ...replaceState... } — seed the state when there is none. Here it becomes:
const noSetHistory = !window.history.state;
if (!noSetHistory) { // === if (window.history.state)So the seeding block now runs only when state already exists (where it's redundant) and is skipped on a cold load (where it's needed). On a fresh page load history.state stays null, onStateChange reads history.state ?? {} → {}, and the product / component query params are silently dropped.
What this breaks:
- The link this PR adds in
products.html.tmpl—enter_bug.cgi?format=guided&product=Focus&component=Security%3A+iOS("use this form for security issues") — lands on the product picker instead.enter_bug_start(Extension.pm:47) serves the guided template regardless ofproduct, so this JS is the only thing that can honour it. - Any existing bookmark or external link to
enter_bug.cgi?format=guided&product=X. - Reloading mid-flow loses your place.
setStep(undefined)falls through todefault:, which recurses assetStep(this.defaultStep)without forwardingnoSetHistory— so a spuriouspushStatefires on every page load and Back needs two presses to leave the page.
Suggested fix: if (noSetHistory) { ...replaceState... }, and pass noSetHistory through the default: branch of setStep.
Separately, master's legacy #h=step|product|component hash parsing was dropped here. extensions/BMO/template/en/default/bug/create/create-mdn.html.tmpl:198 still emits one of those links.
|
|
||
| var name = this.getName(); | ||
| result.push(name); | ||
| return [productName, ...(products[productName].related ?? [])]; |
There was a problem hiding this comment.
Blocking — throws for any product not listed in products.js.
products[productName] is undefined for every product that isn't one of the ~8 entries in products.js, so .related raises a TypeError. master guarded this:
if (products[name] && products[name].related) { ... }This getter is evaluated inside doSearch's inner try (line ~767), so the throw is swallowed into data = { error: true } and the user just sees "An error occurred while searching for similar issues". In other words duplicate detection is dead for every product reached through Other Products — i.e. most of BMO.
It also hits a path this PR touches directly: Core was removed from products.js in this PR, but the webdev step still selects it (product_name="Core" in guided.html.tmpl).
| return [productName, ...(products[productName].related ?? [])]; | |
| return [productName, ...(products[productName]?.related ?? [])]; |
| document.getElementById(id).classList.remove('missing'); | ||
| } | ||
|
|
||
| new Bugzilla.AttachmentSelector({ |
There was a problem hiding this comment.
Blocking — a new AttachmentSelector is constructed on every onShow(), silently dropping the user's attachment.
onShow() runs on every transition into the form step, including popstate. Each construction calls #renderUI(), which does $placeholder.innerHTML = ... and destroys #att-data (the base64 payload) and #att-filename.
Repro: attach a file → click Change next to Product → pick the product again → the file is gone, but #att-desc-section is still visible with the stale filename sitting in description.
Each pass also leaks a submit listener on #bug-form plus a click listener on the submit button (attachment.js:406), which are never removed and end up operating on detached nodes.
Suggested fix — construct it once:
this.selector ??= new Bugzilla.AttachmentSelector({ ... });| document.querySelector('#att-dropbox')?.classList | ||
| .toggle('attention', invalid); | ||
| } else if ($input.type === 'text') { | ||
| if ($input.type === 'text') { |
There was a problem hiding this comment.
Blocking — removing the #att-data branch means a required attachment no longer blocks submit on the BugModal New Bug form.
The deleted branch was the only thing enforcing this here, and AttachmentSelector didn't carry over master's update_validation() / setCustomValidity(). The replacement (AttachmentSelector.validate) hooks the form's submit event and the submit button's click — but neither fires on this path:
$formis a native element (const $form = document.querySelector('#changeform'), line 119), and this handler ends in$form.submit()(line ~804).HTMLFormElement.submit()fires nosubmitevent and skips constraint validation entirely.editModeistrueon the create page (let editMode = isNewBug || false, line 138), so that's the branch taken.- The
clicklistener does run, but this handler already callsevent.preventDefault()unconditionally and then proceeds regardless of what other listeners did.
Repro: Attach New File → fill in a Description → leave the dropbox empty → Submit. "You must provide an attachment." appears and the bug is created anyway, with no attachment.
Either restore an equivalent check here, or have AttachmentSelector reinstate setCustomValidity() so $form.checkValidity() on line 784 catches it.
|
|
||
| this.$description.select(); | ||
| this.$description.focus(); | ||
| this.showPreview(file, isText); |
There was a problem hiding this comment.
An oversized file shows a preview and then files a bug with no attachment.
When checkFileSize fails, $data / $filename are cleared — but showPreview(file, isText) runs unconditionally, so the user sees a preview and reasonably assumes the file is attached.
master compensated with this.$data.required = true (old attachment.js:435), which blocked native form submission. That's gone, and on any form where the attachment is optional (guided form, BugModal/standard New Bug without "wants attachment") validate() computes invalid === false and actively wipes the size error on its way through:
this.$errorMessage.textContent = invalid ? '...' : '';
this.$errorMessage.hidden = !invalid;
this.$dropbox.classList.toggle('invalid', invalid);So the warning disappears at submit time and the bug is filed with nothing attached. Suggest skipping showPreview (or clearing it) when checkFileSize returns false, and not clearing an existing size error in validate().
| } catch(ex) { | ||
| // IE only | ||
| elVersions.add(opt, 0); | ||
| // Fallback to 'unspecified' if available |
There was a problem hiding this comment.
Behaviour change worth a second look: when no version was detected and there's no unspecified option, the <select> keeps its implicit first-option selection, and onVersionChange($versions.value) a few lines down writes that into the hidden version field. checkMissing then sees a non-empty value, so the bug is filed against an arbitrary version the user never picked.
master inserted a blank option and set value = '' specifically to force a decision here. Suggest restoring that.
Related: #version-section is hidden with the hidden attribute, but requiredFields filters on :not([aria-hidden="true"]). If a product ever has no active versions, the hidden-and-empty select fails validation and focus() on it is a no-op — the form silently refuses to submit with nothing shown to the user.
| */ | ||
| static showProductSupport() { | ||
| const { productName } = GuidedBugEntryProductPage; | ||
| const elSupportId = `product-support-${productName.replace(' ', '-').toLowerCase()}`; |
There was a problem hiding this comment.
Dead code carried over from master — nothing has a product-support class. The template has id="product-support" (singular element), and the real show/hide logic lives in setProduct() at line ~371.
Safe to delete this method along with its callers in reset() and onShow().
| <input id="att-filename" type="hidden" name="filename"> | ||
| <textarea hidden id="att-data" name="data_base64" aria-hidden="true" | ||
| aria-invalid="false" aria-errormessage="att-data-error"></textarea> | ||
| <figure role="img" aria-labelledby="att-preview-name" itemscope |
There was a problem hiding this comment.
Two dangling ARIA references in the generated markup:
aria-labelledby="att-preview-name"— line 361 gives the figcaptionclass="att-preview-name", notid.masterusedid="att-preview-name", so the figure has lost its accessible name.- Line 355's
aria-errormessage="att-data-error"points at a<div id="att-data-error">that this PR deletes fromcreateformcontents.html.tmpl.
Either restore the id on the figcaption and re-add the error div, or drop the two ARIA attributes.
| </span> | ||
| [% END %] | ||
| <li role="button" tabindex="0" class="product-item product-link" | ||
| data-link="[% link FILTER none %]" data-product="[% product_name FILTER html %]" |
There was a problem hiding this comment.
Nit / defence in depth: FILTER none on an attribute value that feeds a navigation sink. guided.js:186 does location.href = link with this value, so a javascript: URL here would execute.
Every link today is a template literal from products.html.tmpl, so this isn't exploitable as written — but it's one dynamic value away from being an XSS. Suggest FILTER html here plus a scheme check in the JS before assigning to location.href.
| </div> | ||
| </div> | ||
| </header> | ||
| <div id="steps" hidden> |
There was a problem hiding this comment.
The <noscript> block from master was dropped. #steps is hidden and only JS unhides it, so with JS disabled users now get a page header, a stepper and a footer link with no explanation of why the form is missing.
master pointed them at enter_bug.cgi?format=__default__. Cheap to keep — worth re-adding unless it was deliberate.
Bug 1995464 - Overhaul guided bug entry form
Apologies for the long delay. 🙇🏼 🙇🏼 🙇🏼 Here’s a complete overhaul of the guided bug entry form.
These features that require backend changes are missing in this PR. I’ll follow up later.